Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BackingImage does not download URL correctly in some situation #182

Merged
merged 1 commit into from
Feb 21, 2024
Merged

BackingImage does not download URL correctly in some situation #182

merged 1 commit into from
Feb 21, 2024

Conversation

votdev
Copy link
Contributor

@votdev votdev commented Feb 12, 2024 •
edited
Loading

Remove the Referer header that is automatically added by the http client. Without this header it is possible to download files that are redirected several times. This mimics the behaviour of curl and wget which do not submit the Referer header by default.

Which issue(s) this PR fixes:

longhorn/longhorn#7914

What this PR does / why we need it:

This PR removes the Referer header from the requests. This is necessary to allow downloads of files (e.g. ISO images) that are delivered from file hosters (e.g. SourceForge) via CDN which normally results in several redirects until the requested file is downloaded.

This PR mimics the default behaviour of tools like curl or wget.

@votdev votdev requested a review from a team as a code owner February 12, 2024 13:41
@votdev votdev self-assigned this Feb 12, 2024
@votdev votdev added the enhancement New feature or request label Feb 12, 2024
@votdev votdev force-pushed the issue_7914_rm_referer_header branch 3 times, most recently from 0357c13 to 45b6147 Compare February 12, 2024 16:24
@votdev
Copy link
Contributor Author

votdev commented Feb 13, 2024

Testplan

  1. Create a file with the following content:
apiVersion: longhorn.io/v1beta2
kind: BackingImage
metadata:
  name: clonezilla
  namespace: longhorn-system
spec:
  sourceParameters:
    url: https://sourceforge.net/projects/clonezilla/files/clonezilla_live_alternative/20240116-mantic/clonezilla-live-20240116-mantic-amd64.iso
  sourceType: download
  1. Apply it with k apply -f clonezilla.yaml.
  2. Check the output:
$ k get backingimage -n longhorn-system
NAME                  UUID       SOURCETYPE   SIZE        AGE
clonezilla            42549254   download     102954      11s

This is the current state. The ISO is not downloaded properly.
4. Install the backing-image-manager image containing this PR.
5. Delete the clonezilla backing image via UI.
6. Apply it with k apply -f clonezilla.yaml.
7. Check the output:

$ k get backingimage -n longhorn-system
NAME                  UUID       SOURCETYPE   SIZE        AGE
clonezilla            9c339f0f   download     500170752   8s
  1. Create another backing image.
apiVersion: longhorn.io/v1beta2
kind: BackingImage
metadata:
  name: systemrescue
  namespace: longhorn-system
spec:
  sourceParameters:
    url: https://fastly-cdn.system-rescue.org/releases/11.00/systemrescue-11.00-amd64.iso
  sourceType: download
  1. Check the output:
$ k get backingimage -n longhorn-system
NAME                  UUID       SOURCETYPE   SIZE        AGE
systemrescue          28b76870   download     894435328   17s

@votdev votdev force-pushed the issue_7914_rm_referer_header branch from 45b6147 to 8dddb01 Compare February 15, 2024 16:14
@votdev
BackingImage does not download URL correctly in some situation
e5a5c24 
Remove the `Referer` header that is automatically added by the http client. Without this header it is possible to download files that are redirected several times. This mimics the behaviour of `curl` and `wget` which do not submit the `Referer` header by default.

Signed-off-by: Volker Theile <vtheile@suse.com>
@votdev votdev force-pushed the issue_7914_rm_referer_header branch from 8dddb01 to e5a5c24 Compare February 16, 2024 14:07
ChanYiLin
ChanYiLin approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@ChanYiLin ChanYiLin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the contribution!

@innobead innobead merged commit df58873 into longhorn:master Feb 21, 2024
6 checks passed
@innobead
Copy link
Member

@mergify backport v1.6.x v1.5.x

@mergify Mergify
Copy link

mergify bot commented Feb 21, 2024 •
edited
Loading

backport v1.6.x v1.5.x

✅ Backports have been created

This was referenced Feb 21, 2024
Merged
Merged
@yangchiu yangchiu mentioned this pull request Feb 21, 2024
Closed
@yangchiu yangchiu mentioned this pull request Mar 1, 2024
Closed
@votdev votdev deleted the issue_7914_rm_referer_header branch March 4, 2024 08:33
@yangchiu yangchiu mentioned this pull request Mar 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChanYiLin ChanYiLin ChanYiLin approved these changes

@innobead innobead innobead approved these changes

@FrankYang0529 FrankYang0529 Awaiting requested review from FrankYang0529

Assignees

@votdev votdev

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@votdev @innobead @ChanYiLin